Implements handling of insecure TLS connections in Mehkit #373
Conversation
Signed-off-by: yash37158 <[email protected]>
|
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack. |
|
Whoo-hoo, @yash37158 👍 |
There was a problem hiding this comment.
@yash37158 The current behaviour is if you provide the flag but your kube config has the
certificate-authority-data for the cluster the client-go (k8s package) will ignore the flag, were as if you remove the certificate-authority-data and then use the flag, it will be respected.
This behaviour is consistent with kubectl as well as k8s-client-go.
So I don't think this change is required Or you can do it in this way, if the flag is present remove the certificate-authority-data.
@MUzairS15 i don't think its best he removes the |
There was a problem hiding this comment.
@yash37158 kindly work on your indents and newline spacing, meshkit is very sensitive to this.
|
@Philip-21 I agree, but the testcases he have, doesn’t include certificate-data hence the tests passes, (which comply with the default behaviour of client-go) |
Alright . So what do you think he should do, should he leave the way it was or he should make the certificate-data. Which is better |
|
I appreciate everyone's review and suggestions. I'm working on the changes and will push them as soon as possible. |
|
@MUzairS15, if removing certificate-authority-data is a good approach as Philips said removing may affect the mesheryctl operations. how should i go about this ? |
Signed-off-by: yash37158 <[email protected]>
merging into master
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: yash37158 <[email protected]>
|
Hey @Philip-21 @MUzairS15, I have made the necessary changes suggested by @MUzairS15 |
There was a problem hiding this comment.
@yash37158 Thanks for working on this, going through reviews and making changes.
The proposed fix adds convenience, but because most of the users will be accustomed to what is already present in k8s The merge of this PR would require a PR explaining this behaviour in meshery docs.
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Co-authored-by: Aisuko <[email protected]> Signed-off-by: Yash Sharma <[email protected]>
Co-authored-by: Aisuko <[email protected]> Signed-off-by: Yash Sharma <[email protected]>
|
Thanks for your review @Aisuko I have added the changes you asked for. are we good to merge this now? |
|
hi @yash37158 , It looks like we need to sign off https://github.com/meshery/meshkit/pull/373/checks?check_run_id=18097641313 |
|
Hey @Aisuko, I already signoff my latest commit. I have added your suggestion from the github itself. below is the screenshot attached. where did I go wrong? |
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: yash37158 <[email protected]>
Signed-off-by: yash37158 <[email protected]>
|
@Aisuko, Are we good now? |
Description
This PR fixes #8619
Implements handling of insecure TLS connections in Meshkit.
When connecting to Kubernetes clusters with a kubeconfig that has the insecure-skip-tls-verify: true setting configured, Meshery Server will now respect that setting and ignore TLS/SSL issues.
This feature is implemented in MeshKit and includes the following acceptance tests:
Meshkit now enforces or ignores SSL errors on a per Kubernetes context basis, allowing import of a multi-context
kubeconfig with the same or different insecure-skip-tls-verify settings per context.
A manifest of all attempted, ignored, and successful connections are returned to the client in a Meshery Event (/api/events). But I still need a better understanding of how can i achieve this, on digging I came to know that Meshery uses pub/sub for passing events. but I am not sure if that is correct also how can I return it client(/api/event) and populate the ui? if someone would like to contribute or have a better approach for passing the status to the client.
Here is the test results for the desired state:
Notes for Reviewers
Signed commits